Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1783 #1784

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fix #1783 #1784

wants to merge 11 commits into from

Conversation

zg009
Copy link
Contributor

@zg009 zg009 commented May 11, 2024

This is the thread for addressing #1783

Current has some work to register hostname on server creation and apply it to AccountTemplate class as a static variable

Ran some tests with localhost webId resolution.

Integration tests probably need more granularity, also strategy to handle multiuser mapping due to prefix path rather than postfix.

@zg009
Copy link
Contributor Author

zg009 commented May 11, 2024

@jeff-zucker If you'd like to pull this branch and see if it fulfills your needs, let me know. I can fix issues as they appear or don't resolve properly, but this will serve as the starting point.

@jeff-zucker
Copy link
Member

@jeff-zucker If you'd like to pull this branch and see if it fulfills your needs, let me know. I can fix issues as they appear or don't resolve properly, but this will serve as the starting point.

The code looks right. I'll download and test tomorrow. Thanks much!

@bourgeoa
Copy link
Member

@zg009 Thanks. I think this is correct.
You may make :

  • some cleaning in static registerHostName()
  • minor but I would rename realWebId --> webIdPathToPod or podRelativeWebId or webIdRelToPodRoot
    not easy to explain. The intention is to get the WebId relative to pod Root
  • your tests are OK but do not show that except external Webid in NSS the resulting relative WebId will be --> /profile/card#me

@zg009
Copy link
Contributor Author

zg009 commented May 13, 2024

@bourgeoa I addressed your first and second point. I need more clarity about what I need to check in the test suite.

@bourgeoa
Copy link
Member

Your code works but I think (to be discussed) it does not respect the actual code logic (I did not create it nor help maintain this part)

I thin k your function should be implemented here

static from (options) {

To avoid

@zg009
Copy link
Contributor Author

zg009 commented May 13, 2024

@bourgeoa Are you saying in the static from method, move the logic from current changes into the from method and set the 'host' variable in that way?

@bourgeoa
Copy link
Member

Yes but I am not sure of me at all

@bourgeoa
Copy link
Member

I was thinking that from was returning all data needed from input

@zg009
Copy link
Contributor Author

zg009 commented May 13, 2024

The only places I see AccountManager and AccountTemplate interacting is here

createAccountFor (userAccount) {
const template = AccountTemplate.for(userAccount)
const templatePath = this.accountTemplatePath
const accountDir = this.accountDirFor(userAccount.username)
debug(`Creating account folder for ${userAccount.webId} at ${accountDir}`)
return AccountTemplate.copyTemplateDir(templatePath, accountDir)
.then(() => {
return template.processAccount(accountDir)
})
}

If you trace AccountTemplate.for(), it is the only code which uses AccountTemplate.templateSubstitutionsFor().

I could modify the AccountTemplate.for method to take hostname from AccountManager object from AccountManager.createAccountFor.

EDIT: Actually, looking at the code further, the same end goal is reached. I do not have strong feelings about where to move the host server checking logic. It seems logical to pass hostname to AccountTemplate.for and change webId path based upon that.

@bourgeoa
Copy link
Member

To improve cleaning the logic this should be moved :

AccountTemplate.registerHostname(argv.serverUri)

inside function initWebId

And may be passed via accountManager with a new parameter argv.serverUri to the API.accounts.middleware()

app.use('/', API.accounts.middleware(accountManager))

@bourgeoa
Copy link
Member

I wonder if there is a situation where there is any difference between registerHostname() and

accountUriFor (accountName) {

What do you think ? May be with external WebId ?

@bourgeoa bourgeoa marked this pull request as draft May 16, 2024 17:00
@bourgeoa
Copy link
Member

I marked it as draft because it is not working.
I think the logic is more complex between

  • the user form to create a userAccount
  • the userAccount going to create the pod using the templates

The podUri is not stored in userAccount. May be look at rootAclUri

@zg009
Copy link
Contributor Author

zg009 commented May 16, 2024

Yeah, I figured this wouldn't be so simple. I'll have some time to take a deeper dive later this week or early June, I am busy with my current work right now.

@bourgeoa
Copy link
Member

It is the first time I go in the logic of the account manager of NSS code.
Anyway thanks for looking.

@jeff-zucker
Copy link
Member

Yeah, I figured this wouldn't be so simple. I'll have some time to take a deeper dive later this week or early June, I am busy with my current work right now.

Thanks much for looking in to it. I do not see this as in any way high priority and can deal with it in other ways, so there is no rush on my account. And I can't think that's priority for others either.

@bourgeoa
Copy link
Member

@jeff-zucker I agree it is not high priority.
But when you get in, and thinks it is quite simple and don't find the code logic you somehow are frustrated.

@zg009 I think I resolved it. Thanks again for your help.
Could you :

  • replace failing tests and add any needed ones
  • /profile/card#me should use predefined constants

@zg009
Copy link
Contributor Author

zg009 commented May 18, 2024

@jeff-zucker I agree it is not high priority. But when you get in, and thinks it is quite simple and don't find the code logic you somehow are frustrated.

@zg009 I think I resolved it. Thanks again for your help. Could you :

  • replace failing tests and add any needed ones
  • /profile/card#me should use predefined constants

You will need to clarify what predefined constants I should be using for these tests.

@bourgeoa
Copy link
Member

This is not in the tests, but to improve the code to use

this.pathCard = options.pathCard || 'profile/card'
this.suffixURI = options.suffixURI || '#me'

in place of /profile/card#me

@zg009
Copy link
Contributor Author

zg009 commented May 21, 2024

@bourgeoa I edited your function a little bit because the webID document was including port. If this is unintended behavior I can revert. I also added one test case. If you want to see other test cases, let me know.

@jeff-zucker
Copy link
Member

@bourgeoa I edited your function a little bit because the webID document was including port.

Yes, this is important, users ought to be able to change the port of the server without having to recreate an account.

@bourgeoa bourgeoa self-assigned this May 22, 2024
@bourgeoa bourgeoa marked this pull request as ready for review May 22, 2024 17:33
@bourgeoa
Copy link
Member

replaced wirh url.origin and updated tests that should have failed on relative webId and did not.

@zg009
Copy link
Contributor Author

zg009 commented May 22, 2024

@bourgeoa Looks better to me, I had the wrong idea about which tests should pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants